Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Test across Node 16/18/20 + arm64 MacOS test #444

Merged
merged 13 commits into from
May 12, 2023

Conversation

YOU54F
Copy link
Member

@YOU54F YOU54F commented May 2, 2023

  • Consolidates CI test task to use single format for all three OS's
  • Updates CI test task to use latest action tags (v3 checkout, v3 node)
  • Remove steps no longer needed (updating node-gyp)
  • Ensure FFI folder is deleted when running download-libs.sh otherwise it asks the user to confirm everytime. 🤯
  • Add a developer note to download the libs, before running npm ci when running locally
  • Add macOS arm64/m1 runner test via Cirrus-CI
    • Requires rosetta due to using v1.x of pact-ruby-standalone
  • Add amd64/x86_64 linux test via Cirrus-CI in preparation for ARM64 linux testing in further PR to be raised
  • Test ARM64 Linux, fails as expected as per issue Linux/arm64 CI failing - Cannot find binary for platform linux with architecture arm64 #416

Smells

Cross CI provider and cross arch has shown a few inconsistencies in content-type matching, which is clearly demonstrable here. Haven't thought of a solution just yet, but this at least highlights and allows us to test more combinations than before

const isWin = process.platform === 'win32';
const isLinux = process.platform === 'linux';
const isDarwinArm64 = process.platform === 'darwin' && process.arch === 'arm64';
const isLinuxArm64 = process.platform === 'linux' && process.arch === 'arm64';
const isCirrusCi = process.env['CIRRUS_CI'] === 'true';
const usesOctetStream =
  isLinuxArm64 || isWin || isDarwinArm64 || (isCirrusCi && isLinux);

See pact-foundation/pact-reference#171 for why we have an OS switch here

Previous state

Windows: does not have magic mime matcher, uses content-type
OSX on CI: does not magic mime matcher, uses content-type
OSX: has magic mime matcher, sniffs content
Linux: has magic mime matcher, sniffs content

Current state

Windows: does not have magic mime matcher, uses content-type
OSX arm64: does not magic mime matcher, uses content-type
OSX x86_64: has magic mime matcher, sniffs content
OSX x86_64 - CI GitHub Actions: has magic mime matcher, sniffs content
Linux - CI Cirrus: does not have magic mime matcher, uses content-type

Copy link
Contributor

@TimothyJones TimothyJones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Left a bunch of comments + suggestions.

.cirrus.yml Show resolved Hide resolved
@@ -8,64 +8,30 @@ on:

jobs:
build-and-test-osx:
runs-on: macos-latest
runs-on: ${{ matrix.os }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AWESOME!! I had been meaning to do this for ages, but never got around to figuring out what the syntax was.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been making some mental foo of late, you can generate matrices dynamically in your actions, so I have a run of pact ffi/ pact verifier cli and pact mock server cli, for all the targets, in one super clean action.

I just wish you could report on individual matrix combos, because you could build up an awesome compat matrix, with github badges.

The other way is to have separate workflows , I need to check if you can pull in one master workflow and then have loads of mini workflows, that just specify x combo (node 14 / windows) so they appear as seperate workflows, and you could get a status badge for each one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wish you could report on individual matrix combos, because you could build up an awesome compat matrix, with github badges.

I dunno - I think it's better to just report the whole success / failure - as then you don't get tempted to continue with "well, it doesn't work on everything we deploy to".

It would be nice to be able to generate the list of tested combinations, though!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a whole just fail isn’t helpful for me finding the needle in the haystack and github’s view of multi matrix runs needs some work, detail gets lots and you just have 12 boxes that al look the same from the truncated description until you click on one, and that is an expensive use of time

i need to check out github blocks again as there is meant to be some cool stuff you can do in readmes now

.github/workflows/build-and-test.yml Show resolved Hide resolved
.github/workflows/build-and-test.yml Show resolved Hide resolved
.github/workflows/build-and-test.yml Show resolved Hide resolved
DEVELOPER.md Outdated Show resolved Hide resolved
script/lib/download-ffi.sh Show resolved Hide resolved
test/message.integration.spec.ts Show resolved Hide resolved
@YOU54F
Copy link
Member Author

YOU54F commented May 3, 2023

Nice work! Left a bunch of comments + suggestions.

appreciate the 👀 buddy ❤️

@YOU54F YOU54F merged commit dac0e46 into pact-foundation:master May 12, 2023
const isDarwinArm64 = process.platform === 'darwin' && process.arch === 'arm64';
const isLinuxArm64 = process.platform === 'linux' && process.arch === 'arm64';
const isCirrusCi = process.env['CIRRUS_CI'] === 'true';
const usesOctetStream =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me sad. Not your fault of course. We need to find a solution to this!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, I'll card this up for a yak shave, now have we have a way of testing repeatably across platform, we can sort it, or document it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants